HYPERFLEET-1274 - refactor: move list parameter parsing to handler layer by kuudori · Pull Request #307 · openshift-hyperfleet/hyperfleet-api

kuudori · 2026-07-21T20:50:23Z

Summary

Extract query parameter parsing from pkg/services/types.go into pkg/handlers/list_params.go, aligning with the handler-validates, service-executes pattern
Use go-playground/validator for struct-level validation with structured error details via ValidationWithDetails
Remove redundant defensive size checks from the service layer (generic.go) while keeping a Size <= 0 guard
Add comprehensive table-driven tests covering defaults, custom values, format errors, range errors, ref pairing, and edge cases

Test plan

make verify-all passes (vet + lint + unit tests)
make test-integration passes
Verify list endpoints return correct pagination defaults (page=1, size=20, order=created_time desc)
Verify validation errors return 400 with structured details array
Verify ?fields=name response includes id field automatically
Verify ?ref_type=dep without ?ref_target_id returns validation error

openshift-ci · 2026-07-21T20:50:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kuudori for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coderabbitai · 2026-07-21T20:50:46Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 41665672-ff54-456e-b62c-8e232d2b8d4b

📥 Commits

Reviewing files that changed from the base of the PR and between c0f7ad0 and 47e3fa1.

📒 Files selected for processing (6)

pkg/api/metadata_types.go
pkg/handlers/list_params.go
pkg/services/adapter_status.go
pkg/services/generic.go
pkg/services/generic_test.go
pkg/services/types.go

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

openshift-hyperfleet/architecture (manual)
openshift-hyperfleet/hyperfleet-api (manual)
openshift-hyperfleet/hyperfleet-sentinel (manual)
openshift-hyperfleet/hyperfleet-adapter (manual)
openshift-hyperfleet/hyperfleet-broker (manual)

🚧 Files skipped from review as they are similar to previous changes (2)

pkg/handlers/list_params.go
pkg/services/types.go

📝 Walkthrough

Summary by CodeRabbit

New Features
- Added consistent list query parsing and validation (pagination, search, fields, ordering, and reference filtering) across all resource list endpoints.
- Fields selection now automatically includes the resource ID and applies default ordering when none is provided.
Bug Fixes
- Improved handling of invalid/incomplete list parameters with clearer validation feedback.
- Ensured consistent field-filtering behavior across endpoints.
Documentation
- Updated mock generation instructions for the current development workflow.
Other
- Paging metadata now uses 64-bit page numbers.

Walkthrough

List query parsing and validation move from pkg/services into pkg/handlers, covering pagination, search, ordering, fields, and reference-pair validation. List endpoints use the new parser, while services retain parameterless defaults, use 64-bit page values, and update list execution to use DAO values and reject non-positive sizes. Field filtering reuses handler normalization helpers, and mock-generation documentation is updated.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ResourceHandler
  participant parseListParams
  participant sqlResourceService
  Client->>ResourceHandler: Send list request with query parameters
  ResourceHandler->>parseListParams: Parse and validate URL query
  parseListParams-->>ResourceHandler: Return ListArguments
  ResourceHandler->>sqlResourceService: Execute list operation
  sqlResourceService-->>ResourceHandler: Return list results

Suggested reviewers: ciaranroche, mliptak0, rh-amarin

🚥 Pre-merge checks | ✅ 9 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
No Injection Vectors	❓ Inconclusive	placeholder	pending

✅ Passed checks (9 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the main change: moving list parameter parsing from services into handlers.
Description check	✅ Passed	The description is aligned with the changeset and accurately covers parsing, validation, service cleanup, and tests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Sec-02: Secrets In Log Output	✅ Passed	No non-test/non-example Go log statements mention token/password/credential/secret; only comments/docs matched. CWE-532 not triggered.
No Hardcoded Secrets	✅ Passed	No CWE-798/CWE-259 indicators in changed files; no hardcoded creds, creds-in-URL, or long base64 literals found, only doc examples.
No Weak Cryptography	✅ Passed	No crypto imports/usages, SHA1/MD5/DES/RC4/ECB, or secret comparisons found in the changed files; no CWE-327/CWE-328/CWE-208 issue.
No Privileged Containers	✅ Passed	No PR changes add privileged pod settings; chart defaults are non-root with allowPrivilegeEscalation:false and drop ALL, and Dockerfile USER root is build-stage only with justification (CWE-250 not...
No Pii Or Sensitive Data In Logs	✅ Passed	No new slog/logr/zap/fmt.Print logging paths were added; the only logging diff removes existing generic.go warnings (CWE-532).

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

✨ Simplify code

Create PR with simplified code

_{Comment @coderabbitai help to get the list of available commands.}

hyperfleet-ci-bot · 2026-07-21T20:53:06Z

Risk Score: 3 — `risk/medium`

Signal	Detail	Points
PR size	1094 lines (>500)	+2
Sensitive paths	none	+0
Test coverage	Missing tests for: pkg/api	+1

_{Computed by hyperfleet-risk-scorer}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/handlers/list_params_test.go (1)
246-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing coverage for the page upper bound.

Once Page gains a max (see pkg/handlers/list_params.go Line 20-21), add a range-error case (e.g. ?page=99999999999999999999) so the overflow guard is regression-locked. Error paths are otherwise well covered.

As per path instructions: "Error paths SHOULD be tested, not just happy paths".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/handlers/list_params_test.go` around lines 246 - 255, Add a table-driven
test case in the list-parameter validation tests covering a page value above the
`Page` maximum, such as an excessively large numeric page query, and assert the
expected range-validation error. Keep the existing valid large-page case
unchanged and use the current error assertion pattern.
Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/handlers/list_params.go`:
- Around line 20-21: Cap Page or explicitly validate its multiplication with
Size before loadList computes the offset and calls Fetch. Update the Page
validation in list parameters and ensure (args.Page-1)*int(args.Size) cannot
overflow into a negative offset, while preserving valid pagination behavior.

---

Nitpick comments:
In `@pkg/handlers/list_params_test.go`:
- Around line 246-255: Add a table-driven test case in the list-parameter
validation tests covering a page value above the `Page` maximum, such as an
excessively large numeric page query, and assert the expected range-validation
error. Keep the existing valid large-page case unchanged and use the current
error assertion pattern.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 15f509e2-4d57-4412-9603-b9ad8c7ae08f

📥 Commits

Reviewing files that changed from the base of the PR and between 99c22d1 and aa6578b.

📒 Files selected for processing (11)

docs/development.md
pkg/handlers/framework.go
pkg/handlers/list_params.go
pkg/handlers/list_params_test.go
pkg/handlers/resource_handler.go
pkg/handlers/resource_status_handler.go
pkg/handlers/root_resource_handler.go
pkg/services/generic.go
pkg/services/resource.go
pkg/services/types.go
pkg/services/types_test.go

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

openshift-hyperfleet/architecture (manual)
openshift-hyperfleet/hyperfleet-api (manual)
openshift-hyperfleet/hyperfleet-sentinel (manual)
openshift-hyperfleet/hyperfleet-adapter (manual)
openshift-hyperfleet/hyperfleet-broker (manual)

💤 Files with no reviewable changes (1)

pkg/services/types_test.go

Extract query parameter parsing from pkg/services/types.go into pkg/handlers/list_params.go. Use go-playground/validator for struct-level validation and return structured error details via ValidationWithDetails. Remove defensive size checks from generic.go service layer (now validated at the handler boundary) while keeping a guard for Size <= 0. Update stale mockgen example in docs/development.md.

…int64

openshift-ci Bot requested review from ciaranRoche and mliptak0 July 21, 2026 20:50

hyperfleet-ci-bot Bot added the risk/medium label Jul 21, 2026

coderabbitai Bot reviewed Jul 21, 2026

View reviewed changes

Comment thread pkg/handlers/list_params.go Outdated

kuudori force-pushed the HYPERFLEET-1274 branch from aa6578b to c0f7ad0 Compare July 22, 2026 16:50

HYPERFLEET-1274 - fix: remove pointer-to-interface and align Page to …

47e3fa1

…int64

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HYPERFLEET-1274 - refactor: move list parameter parsing to handler layer#307

HYPERFLEET-1274 - refactor: move list parameter parsing to handler layer#307
kuudori wants to merge 2 commits into
openshift-hyperfleet:mainfrom
kuudori:HYPERFLEET-1274

kuudori commented Jul 21, 2026

Uh oh!

openshift-ci Bot commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

Summary by CodeRabbit

Walkthrough

Sequence Diagram(s)

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

hyperfleet-ci-bot Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

kuudori commented Jul 21, 2026

Summary

Test plan

Uh oh!

openshift-ci Bot commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Sequence Diagram(s)

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

hyperfleet-ci-bot Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Risk Score: 3 — risk/medium

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

hyperfleet-ci-bot Bot commented Jul 21, 2026 •

edited

Loading

Risk Score: 3 — `risk/medium`